-
Notifications
You must be signed in to change notification settings - Fork 906
[csrng/rtl] Remove four prim_fifo_sync from the data path #28428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d8f932a to
1012a42
Compare
|
Here are the results of a full regression run: CSRNG Simulation ResultsThursday October 30 2025 10:26:27 UTCGitHub Revision:
|
| Stage | Name | Tests | Max Job Runtime | Simulated Time | Passing | Total | Pass Rate |
|---|---|---|---|---|---|---|---|
| V1 | smoke | csrng_smoke | 8.000s | 50.512us | 50 | 50 | 100.00 % |
| V1 | csr_hw_reset | csrng_csr_hw_reset | 2.000s | 281.156us | 5 | 5 | 100.00 % |
| V1 | csr_rw | csrng_csr_rw | 2.000s | 166.236us | 20 | 20 | 100.00 % |
| V1 | csr_bit_bash | csrng_csr_bit_bash | 11.000s | 1.450ms | 5 | 5 | 100.00 % |
| V1 | csr_aliasing | csrng_csr_aliasing | 3.000s | 287.406us | 5 | 5 | 100.00 % |
| V1 | csr_mem_rw_with_rand_reset | csrng_csr_mem_rw_with_rand_reset | 2.000s | 84.225us | 20 | 20 | 100.00 % |
| V1 | regwen_csr_and_corresponding_lockable_csr | csrng_csr_rw | 2.000s | 166.236us | 20 | 20 | 100.00 % |
| csrng_csr_aliasing | 3.000s | 287.406us | 5 | 5 | 100.00 % | ||
| V1 | TOTAL | 105 | 105 | 100.00 % | |||
| V2 | interrupts | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| V2 | alerts | csrng_alert | 25.000s | 4.397ms | 500 | 500 | 100.00 % |
| V2 | err | csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % |
| V2 | cmds | csrng_cmds | 3.167m | 21.760ms | 50 | 50 | 100.00 % |
| V2 | life cycle | csrng_cmds | 3.167m | 21.760ms | 50 | 50 | 100.00 % |
| V2 | stress_all | csrng_stress_all | 12.667m | 107.921ms | 50 | 50 | 100.00 % |
| V2 | intr_test | csrng_intr_test | 1.000s | 48.257us | 50 | 50 | 100.00 % |
| V2 | alert_test | csrng_alert_test | 8.000s | 29.006us | 50 | 50 | 100.00 % |
| V2 | tl_d_oob_addr_access | csrng_tl_errors | 10.000s | 1.869ms | 20 | 20 | 100.00 % |
| V2 | tl_d_illegal_access | csrng_tl_errors | 10.000s | 1.869ms | 20 | 20 | 100.00 % |
| V2 | tl_d_outstanding_access | csrng_csr_hw_reset | 2.000s | 281.156us | 5 | 5 | 100.00 % |
| csrng_csr_rw | 2.000s | 166.236us | 20 | 20 | 100.00 % | ||
| csrng_csr_aliasing | 3.000s | 287.406us | 5 | 5 | 100.00 % | ||
| csrng_same_csr_outstanding | 3.000s | 358.374us | 20 | 20 | 100.00 % | ||
| V2 | tl_d_partial_access | csrng_csr_hw_reset | 2.000s | 281.156us | 5 | 5 | 100.00 % |
| csrng_csr_rw | 2.000s | 166.236us | 20 | 20 | 100.00 % | ||
| csrng_csr_aliasing | 3.000s | 287.406us | 5 | 5 | 100.00 % | ||
| csrng_same_csr_outstanding | 3.000s | 358.374us | 20 | 20 | 100.00 % | ||
| V2 | TOTAL | 1440 | 1440 | 100.00 % | |||
| V2S | tl_intg_err | csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % |
| csrng_tl_intg_err | 9.000s | 1.888ms | 20 | 20 | 100.00 % | ||
| V2S | sec_cm_config_regwen | csrng_regwen | 8.000s | 11.480us | 50 | 50 | 100.00 % |
| csrng_csr_rw | 2.000s | 166.236us | 20 | 20 | 100.00 % | ||
| V2S | sec_cm_config_mubi | csrng_alert | 25.000s | 4.397ms | 500 | 500 | 100.00 % |
| V2S | sec_cm_intersig_mubi | csrng_stress_all | 12.667m | 107.921ms | 50 | 50 | 100.00 % |
| V2S | sec_cm_main_sm_fsm_sparse | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_update_fsm_sparse | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_blk_enc_fsm_sparse | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_outblk_fsm_sparse | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_gen_cmd_ctr_redun | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_drbg_upd_ctr_redun | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_drbg_gen_ctr_redun | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_ctrl_mubi | csrng_alert | 25.000s | 4.397ms | 500 | 500 | 100.00 % |
| V2S | sec_cm_main_sm_ctr_local_esc | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| V2S | sec_cm_constants_lc_gated | csrng_stress_all | 12.667m | 107.921ms | 50 | 50 | 100.00 % |
| V2S | sec_cm_sw_genbits_bus_consistency | csrng_alert | 25.000s | 4.397ms | 500 | 500 | 100.00 % |
| V2S | sec_cm_tile_link_bus_integrity | csrng_tl_intg_err | 9.000s | 1.888ms | 20 | 20 | 100.00 % |
| V2S | sec_cm_aes_cipher_fsm_sparse | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_fsm_redun | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_ctrl_sparse | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_fsm_local_esc | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_ctr_redun | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 8.000s | 72.445us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_data_reg_local_esc | csrng_intr | 11.000s | 448.770us | 200 | 200 | 100.00 % |
| csrng_err | 8.000s | 28.008us | 500 | 500 | 100.00 % | ||
| V2S | TOTAL | 75 | 75 | 100.00 % | |||
| V3 | stress_all_with_rand_reset | csrng_stress_all_with_rand_reset | 3.417m | 31.218ms | 10 | 10 | 100.00 % |
| V3 | TOTAL | 10 | 10 | 100.00 % | |||
| TOTAL | 1630 | 1630 | 100.00 % |
Coverage Results
Coverage Dashboard
| Score | Block | Branch | Statement | Expression | Toggle | Fsm | Assertion | CoverGroup |
|:-------:|:-------:|:--------:|:-----------:|:------------:|:--------:|:--------:|:-----------:|:------------:|
| 97.60 % | 98.65 % | 96.67 % | 99.91 % | 96.89 % | 92.08 % | 100.00 % | 95.81 % | 90.26 % |
INFO: [FlowCfg] [scratch_path]: [csrng] [/scratch/glaserf/opentitan/scratch/csrng_sfifo_removal/csrng-sim-xcelium]
[ legend ]: [Q: queued, D: dispatched, P: passed, F: failed, K: killed, T: total]
00:01:01 [ build ]: [Q: 0, D: 0, P: 2, F: 0, K: 0, T: 2] 100%
00:18:43 [ run ]: [Q: 0, D: 0, P: 1630, F: 0, K: 0, T: 1630] 100%
00:19:12 [ cov_merge ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
00:19:16 [ cov_report ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
1012a42 to
de408f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @glaserf for the PR, this is very nice and clean work!
I have one question. And I think it would be good also run the csrng_kat software test (I am not sure if this is run in CI, too) or just in the regressions.
I think before merging this PR, we should re-assure partners are aware of the version increment and the changes to the software interface.
| fields: [ | ||
| { bits: "7:0", | ||
| { bits: "5:0", | ||
| name: "MAIN_SM_STATE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please grep for MAIN_SM_STATE in the repo and see if there is still software reading this register for properly using CSRNG? I am sure we did this in the past, but I am not sure if this software is run in CI. It might just get run in the nightly / weekly regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, good point! The only thing I could find was a HAL function in the CSRNG dif and a call to it in the dif unit test.
However, the expected value of '42' (hex 0x2A) there does not match the old reset value, and the unit test just after this one, which reads the HW exception status register, also expects 42, so I lean towards the option that this test does not expect the actual reset value of the hardware but just tests the dif itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @glaserf . I think the 42 is just a remainder from a previous version. As you say, this just matters for the DIF unittest.
| assign gen_adata_null_d = !enable_i ? '0 : | ||
| (req_vld_i ? prep_gen_adata_null : gen_adata_null_q); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure but should we use req_vld_i & req_rdy_o here? Otherwise we might clear gen_adata_null_q when seeing a new vld but not yet having finished the last command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I updated the commit in question and also tightened one more handshake condition. Both in theory could lead to problems; however, in reality nothing breaks since the command arbiter/main FSM won't push a new command into the data path before the old one has finished at the state_db side.
I re-ran the full regression and updated the results in the comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
@phani-lowrisc for visibility. |
|
Thanks for the positive feedback, @vogelpi ! Regarding the KAT test, I just browsed the CI jobs that were ran as part of this PR and found the CSRNG KAT in several jobs included and passing, e.g., here: It also passes when ran locally: CHIP Simulation ResultsTuesday October 21 2025 12:46:46 UTCGitHub Revision:
|
| Stage | Name | Tests | Max Job Runtime | Simulated Time | Passing | Total | Pass Rate |
|---|---|---|---|---|---|---|---|
| V2 | chip_sw_csrng_known_answer_tests | chip_sw_csrng_kat_test | 59.000s | 2.592ms | 1 | 1 | 100.00 % |
| V2 | TOTAL | 1 | 1 | 100.00 % | |||
| TOTAL | 1 | 1 | 100.00 % |
INFO: [FlowCfg] [scratch_path]: [chip] [/scratch/glaserf/opentitan/scratch/csrng_sfifo_removal/chip_earlgrey_asic-sim-xcelium]
[ legend ]: [Q: queued, D: dispatched, P: passed, F: failed, K: killed, T: total]
00:00:37 [ build ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
00:01:56 [ run ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
de408f5 to
89786d4
Compare
thanks @vogelpi for adding me will review them as well. |
| MainSmEntropyReq = 6'b001110, // request entropy if necessary | ||
| MainSmCmdPrep = 6'b000011, // delay cycle for command request (?) | ||
| MainSmCmdVld = 6'b010000, // command request to core data path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to explain a mapping of previous states to the new ones. As far as I can tell there used to be multiple prepare and request states that are now combined into prepare and valid states. Something that is new as far as I can tell is the entropy request state. Would you mind adding a description of this mapping in the commit message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I have changed the commit message accordingly. Before, the various request states all asserted a different request signal towards csrng_core, all of which however then got OR-reduced again. Hence, I combined all the different request states into one.
Then, before each request state, there was a matching 'prepare' state, which again, were all very similar and just transitioned to the matching request-state. In addition, some prepare states requested fresh entropy. I therefore created two types of prepare states, one that does request entropy, and one that does not.
|
CHANGE AUTHORIZED: hw/ip/csrng/data/csrng.hjson This PR refactors the CSRNG RTL further to save silicon area. The major version increase is necessary as the software compatibility is theoretically broken with this (FIFO error bits are removed, the meaning of the values readable from the debug register exposing the FSM state changes). The changes are well understood and verified. The pass rates improve and the coverage metrics remain at the previous levels (V2S). We expect moving back to D2S/V2S with a sign off after completing the refactoring. This PR and the version changes have been discussed in the Security WG meeting on 2025-10-23 and in the Silicon and Integrated WG meeting on 2025-10-28. |
89786d4 to
7215edd
Compare
Major version bump, since the following commits introduce breaking changes. Even though the intention is to keep the D2S and V2S stages, the slight required changes to the register map break binary compatibility, which requires a reset to D1 and V1. Signed-off-by: Florian Glaser <[email protected]>
Before this commit, the main state machine had a separate prepare and request state for each command. The prepare states could be grouped into two categories by whether fresh entropy gets requested before moving to the respective request state, or not. Each request state, however, only differed by asserting a different request signal towards csrng_core, which then got OR-combined without any additional security countermeasures. Hence, this commit reduces all request states to a single one (and a single request signal towards csrng_core), and all prepare states to two, one where entropy is requested, and one where this is not done. Furthermore, the commit changes the interaction between main state machine, the command stages and the ctr_drbg_cmd module to be based on proper valid/ready handshakes. Signed-off-by: Florian Glaser <[email protected]>
Remove the cmdreq FIFO in csrng_ctr_drbg_cmd and instead use data from the state db and the adata unpacker. The FIFO in question is the largest in the design, measuring around 9kGE. This commit can be seen as a blueprint for the ones that follow in regards to the removal of the respective error reporting circuitry and bits in the register map, dv, and dif. Signed-off-by: Florian Glaser <[email protected]>
This commit removes the pdata FIFO from the csrng_ctr_drbg_upd stage. As a consquence, pdata for the final step is now taken from the request fifo right at the request port of this module. This requires the internal control logic to wait for the block_encrypt module to process all three requests until the upstream req_rdy_o can be asserted. This change, however, does not reduce performance/throughput of the CSRNG. Signed-off-by: Florian Glaser <[email protected]>
This commit bridges the bencreq FIFO and instead feeds the read data from the updreq FIFO to the block_encrypt. This has neither functional nor timing impacts. Signed-off-by: Florian Glaser <[email protected]>
The input data is now taken directly from either the output FIFO in ctr_drbg_gen or the state db and the adata unpacker, depending on the origin of the request. Signed-off-by: Florian Glaser <[email protected]>
This commit addresses a rare dv failure caused by an incorrect counter width assumed in csrng_err_vseq. Instead, move the counter width definition to csrng_pkg and use use the value from there. Signed-off-by: Florian Glaser <[email protected]>
7215edd to
7aaa093
Compare
|
CHANGE AUTHORIZED: hw/ip/csrng/data/csrng.hjson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHANGE AUTHORIZED: hw/ip/csrng/data/csrng.hjson
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_cmd_stage.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_core.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_ctr_drbg_cmd.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_ctr_drbg_upd.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_main_sm.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_pkg.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_reg_pkg.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_reg_top.sv
This PR marks the beginning of the FIFO removal efforts for the CSRNG data path. In order to make this easier, this PR first simplifies the main FSM by removing states that effectively were identical (without providing any security countermeasures or hardening) and streamlining handshaking with the command stages.
Each of the following commits removes one FIFO; they are all quite similar in their structure/set of files that are affected. Since each FIFO is connected to several error reporting bits in the regfile, documentation,
dif, and dv, these parts of the IP also have to get touched every time a FIFO is removed.Since both the FSM simplification and removal of the FIFO error bits affect the register map, these changes are breaking. The first commit therefore bumps the version to 3.0.0 and resets the design and verification stages to D1 and V1, respectively, even though the intent here is to keep the design at the current D2S and V2S stages.
Once all breaking changes have been implemented and merged (will be distributed over several PRs), we'll hence have to sign-off the IP again.
The proposed changes have no timing impact on the IP; the critical path remains within the AES cipher.
Part of #28153.